Code review

A key point here is that there is no such thing as “perfect” code—there is only better code. Reviewers should not require the author to polish every tiny piece of a CL before granting approval.

Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review.

In the general case, look at every line of code that you have been assigned to review.

100 lines is usually a reasonable size for a CL, and 1000 lines is usually too large, but it’s up to the judgment of your reviewer.

PR practice

レビュー修正が溜まって commit が増えてしまう問題(?) をどうするべきか

fixup を使う

レビューされた後に commit をいじることに不安を覚えるがどうだろうか？

OSS でもマージする前に squash するプロジェクトとかある気がするから別にいいのか

自分の考え

なるべく早く Draft PR を作成

リリースしても良いと思っているコードしかレビュー依頼を出さない

つまりサーバーでの動作確認ができていないコードはレビューにはこない

PR の説明では reviewer に必要な情報をすべて与える

例えば

PR の意図はコードから読み取れないことが多いので説明する

新しく導入するパッケージがある場合はリンク貼る

reviewer が詳しくないと事前に予想がついていることについては参考になるリンクを貼る

特にチェックして欲しいポイント、意見が欲しいポイントがある場合は書いておく

別 PR で対応するものがある場合は書いておく

レビューの最中に不要な force-push をしない

reviewer がローカルに pull してくるのがちょっと手間になる

マージは PR 作成者が行う